Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

STCOR-950 Added location to a hook in <MainNav> to fix comparing old location value #1600

Merged
merged 2 commits into from
Feb 24, 2025
Merged

STCOR-950 Added location to a hook in <MainNav> to fix comparing old location value #1600

merged 2 commits into from
Feb 24, 2025
+2 −1

Conversation

BogdanDenis
Copy link
Contributor

Description

After the switch to hooks, in stripes-core/src/components/MainNav/MainNav.js at 3570eea0bca50bb0abb0cc0e52db3ae4bc3a792e · folio-org/stripes-core location object is not updated, and so store.subscribe callback always uses an old value.

When updateLocation compares cleanStateQuery and cleanLocationQuery objects, they are always different because we’re comparing to an old location

This caused a bug in Inventory, where when a user opens a shared record - the page reloads and user is taken back to search results. This happens because when a user opens a shared record we append shared=true to the url. And because <MainNav> doesn't compare to a current url the flow gets interrupted.

If we add location to the hook’s deps it works as expected.

Screenshots

chrome_ihYEknGL4o.mp4

Issues

STCOR-950

@BogdanDenis BogdanDenis requested a review from JohnC-80 February 21, 2025 15:15
@BogdanDenis BogdanDenis requested a review from a team as a code owner February 21, 2025 15:15
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 21, 2025 •
edited
Loading

Bigtest Unit Test Results

189 tests  ±0   184 ✅ ±0   6s ⏱️ ±0s
  1 suites ±0     5 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 9f30da8. ± Comparison against base commit 18b82a3.

This pull request removes 5 and adds 3 tests. Note that renamed tests count towards both. 
      equal to check email label in english translation
      equal to check email precautions label in english translation
      equal to sent email precautions label in english translation
Chrome_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the header with an appropriate text content
Chrome_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the paragraph with an appropriate text content

Chrome_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the header with an appropriate text content
      equal to check email label in english translation
Chrome_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the paragraph with an appropriate text content
      equal to check email precautions label in english translation
Chrome_133_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the paragraph with an appropriate text content
      equal to sent email precautions label in english translation

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 21, 2025 •
edited
Loading

Jest Unit Test Results

  1 files  ±0   64 suites  ±0   1m 32s ⏱️ -1s
381 tests ±0  381 ✅ ±0  0 💤 ±0  0 ❌ ±0 
385 runs  ±0  385 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 9f30da8. ± Comparison against base commit 18b82a3.

♻️ This comment has been updated with latest results.

zburke
zburke approved these changes Feb 21, 2025
View reviewed changes
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooof, sorry about this, @BogdanDenis . Thank you for fixing it!

@JohnC-80 , we introduced a lot of eslint-disable-line comments in #1584 😬 . Could you write up a ticket to address these concerns:

  1. either annotate them ("here's why we don't need dependencies...") or fix them.
  2. reduce the blanket scope of the disable to react-hooks/exhaustive-deps

We should work hard to avoid disabling lint. Sure, it's super annoying, but it's also smarter than us a lot of the time.

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@BogdanDenis BogdanDenis merged commit d0fca16 into master Feb 24, 2025
16 checks passed
@BogdanDenis BogdanDenis deleted the STCOR-950 branch February 24, 2025 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zburke zburke zburke approved these changes

@JohnC-80 JohnC-80 JohnC-80 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BogdanDenis @zburke @JohnC-80